Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[no squash] Auto-toggle TouchScreenGUI in-game when receiving touch/mouse input #14542

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

grorp
Copy link
Member

@grorp grorp commented Apr 13, 2024

With this PR, the touch controls are automatically created/deleted based on the last used pointer type (mouse or touch).

It is based on #14472 because before that PR, TouchScreenGUI didn't ever delete its IGUIButtons, it just hid them. They were deleted somewhere else on shutdown. While this worked without in-game creation/deletion of TouchScreenGUIs, it would result in memory leaks with this PR.

Note that no automatic switching is done on keyboard input. This is because TouchScreenGUI + keyboard works while TouchScreenGUI + mouse doesn't.

Related: #14400, #14400 (comment), #14400 (comment)

To do

This PR is a Ready for Review.

  • Should automatic switching change the enable_touch setting as well?
  • Touchscreen broken on X11 due to fake mouse events
  • Camera jumps when switching from touchscreen to mouse on Android
  • Code cleanup
  • More testing on different platforms

How to test

Buy an Android phone, a bluetooth mouse and a bluetooth keyboard. See that the touch controls are automatically created/deleted when you use the touchscreen/mouse.

@grorp grorp added Android Feature ✨ PRs that add or enhance a feature @ Client / Controls / Input Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels Apr 13, 2024
@grorp grorp marked this pull request as draft April 13, 2024 17:04
@grorp grorp force-pushed the auto-switch branch 2 times, most recently from e3adfb7 to f4ca1ea Compare April 17, 2024 14:04
@grorp grorp removed the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Apr 17, 2024
@grorp
Copy link
Member Author

grorp commented May 5, 2024

I've finally been able to fix the camera jump issue on Android using two changes from MoNTE48/irrlicht and one own change. Idk why this combination of changes works, but it does work and I've spent too many hours on this bug already.

The remaining hard issue is the "fake mouse events on X11" one.

MouseY = irrevent.MouseInput.Y = SDL_event.motion.y;
} else {
MouseX = irrevent.MouseInput.X = MouseX + SDL_event.motion.xrel;
MouseY = irrevent.MouseInput.Y = MouseY + SDL_event.motion.yrel;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this make a difference on platforms other than Android? at least Multicraft Irrlicht also applies it on all platforms: https://github.com/MoNTE48/Irrlicht/blob/a2910941a0c64e58f294e3e4aa041fb6f7a1db9a/source/Irrlicht/CIrrDeviceSDL.cpp#L856-L865

@grorp grorp force-pushed the auto-switch branch 4 times, most recently from 9d37510 to da9f2ea Compare May 15, 2024 19:55
@grorp grorp changed the title Auto-toggle TouchScreenGUI in-game when receiving touch/mouse input [no squash] Auto-toggle TouchScreenGUI in-game when receiving touch/mouse input May 15, 2024
@grorp
Copy link
Member Author

grorp commented May 15, 2024

This PR now implements the approach suggested by @okias since my own idea didn't work as well:

Phase 1

  • Split touchscreen settings into UI layout and touchscreen controls

Phase 2

  • auto-detect form-factor and set newly created UI layout variable by default regarding to the detected device form-factor

Phase 3

  • switch touchscreen controls option from ON/OFF to AUTO/ON/OFF, where default auto toggles touchscreen controls on touch/mouse input

slightly modified, original by okias in #14400 (comment)

I've also renamed TouchScreenGUI to TouchControls to avoid confusion. rubenwardy has made a similar suggestion in #14075 (comment).

@okias Would you be interested in giving my changes a review? There's still some cleanup of accumulated quick-and-dirty fixes to do, but the PR should be ready in terms of concept and features now.

src/defaultsettings.cpp Outdated Show resolved Hide resolved
@okias
Copy link
Contributor

okias commented May 19, 2024

s/gui_touch/adjust_touchscreen_ui/ or /touchscreen_ui_adjust/... gui_touch feel too confusing.

But I hope after next release there will be rewrite, where UI will become hack-free for touchscreen and just universal code for both desktop and mobile.

to avoid confusion between touchscreen-related settings that affect GUIs
(formspecs) and touchscreen-related settings that affect the touch controls
(TouchControls / formerly TouchScreenGUI)
…ouch_gui

touch_gui provide adjustment to the interface, so it's more touch
friendly

Signed-off-by: David Heidelberg <david@ixit.cz>
@grorp grorp marked this pull request as ready for review June 13, 2024 17:34
@grorp
Copy link
Member Author

grorp commented Jun 13, 2024

A part of this has been split off into #14749. I've rebased this PR on top of that one.

I also did the remaining code cleanup / integration, so this is ready for review now. I quickly tested it on Android and it still seems to work.

@grorp grorp added the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Jun 13, 2024
irr/include/IEventReceiver.h Outdated Show resolved Hide resolved
@@ -344,6 +344,9 @@ struct SEvent

//! Type of mouse event
EMOUSE_INPUT_EVENT Event;

//! Is this a simulated mouse event generated by Minetest itself?
bool Simulated;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could reuse e.g. ButtonStates with a flag that indicates simulated or not
but other than that setting this in the constructor sounds like the correct approach

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could reuse e.g. ButtonStates with a flag that indicates simulated or not

I fear bugs, consider this:

// Mouse wheel and move events: send to hovered element instead of focused
if (event.EventType == EET_MOUSE_INPUT_EVENT &&
(event.MouseInput.Event == EMIE_MOUSE_WHEEL ||
(event.MouseInput.Event == EMIE_MOUSE_MOVED &&
event.MouseInput.ButtonStates == 0))) {

but other than that setting this in the constructor sounds like the correct approach

I now notice that setting it in the constructor probably isn't good either. Consider CIrrDeviceSDL, which reuses one SEvent variable for all Irrlicht events it creates from SDL events.

bool CIrrDeviceSDL::run()
{
os::Timer::tick();
SEvent irrevent;

Since union members other than MouseInput are used on that variable (at least KeyInput & TouchInput), MouseInput.Simulated might (in theory) already be overwritten when a mouse event is created.

Copy link
Member

@sfan5 sfan5 Jun 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now notice that setting it in the constructor probably isn't good either. Consider CIrrDeviceSDL, which reuses one SEvent variable for all Irrlicht events it creates from SDL events.

bad practice IMO, it shouldn't be doing that.
if you want to fix that you could simply insert irrevent = {} before every use

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, shouldn't replacing every occurrence of SEvent event; with SEvent event{}; also work as a solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android @ Client / Controls / Input Feature ✨ PRs that add or enhance a feature Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow switching between touchscreen and other input on Desktop and Android
3 participants